test: add unit tests for IsClusterReady and ClusterAccessCredentialChanged#7677
test: add unit tests for IsClusterReady and ClusterAccessCredentialChanged#7677Goutham-Annem wants to merge 1 commit into
Conversation
…anged Add unit tests for two functions in pkg/util/cluster.go that lacked coverage: - IsClusterReady: covers no conditions, Ready=True, Ready=False, Ready=Unknown, and Ready condition absent - ClusterAccessCredentialChanged: covers identical specs (no change), and each changed field: APIEndpoint, InsecureSkipTLSVerification, ProxyURL, ProxyHeader, plus the empty-specs base case Fixes karmada-io#7676 Signed-off-by: Goutham Annem <annem.usedu@gmail.com>
karmada-bot
added
the
kind/feature
|
Invalid commit message issues detected Invalid commit messagesKeywords which can automatically close issues and hashtag(#) mentions are not allowed.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
karmada-bot
added
the
do-not-merge/invalid-commit-message
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability of the codebase by introducing unit tests for previously uncovered utility functions in pkg/util/cluster.go. These tests validate cluster readiness conditions and credential change detection logic, ensuring robust behavior across different input states. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
karmada-bot
added
the
size/L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces unit tests for IsClusterReady and ClusterAccessCredentialChanged in pkg/util/cluster_test.go. The review feedback suggests adding a test case to ensure changes to non-credential fields do not trigger false positives. Additionally, it points out that the underlying ClusterAccessCredentialChanged function does not check for changes in SecretRef or ImpersonatorSecretRef, which could lead to undetected credential updates.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| { | ||
| name: "empty specs — no change", | ||
| newSpec: clusterv1alpha1.ClusterSpec{}, | ||
| oldSpec: clusterv1alpha1.ClusterSpec{}, | ||
| want: false, | ||
| }, |
There was a problem hiding this comment.
It is important to verify that changes to non-credential fields (such as SyncMode, Provider, Region, etc.) do not trigger a credential change detection. Adding a test case for this ensures that ClusterAccessCredentialChanged only returns true for actual credential-related changes.
{
name: "empty specs — no change",
newSpec: clusterv1alpha1.ClusterSpec{},
oldSpec: clusterv1alpha1.ClusterSpec{},
want: false,
},
{
name: "non-credential fields changed — no change",
newSpec: clusterv1alpha1.ClusterSpec{
APIEndpoint: "https://k8s.example.com",
SyncMode: clusterv1alpha1.Push,
},
oldSpec: clusterv1alpha1.ClusterSpec{
APIEndpoint: "https://k8s.example.com",
SyncMode: clusterv1alpha1.Pull,
},
want: false,
},| } | ||
| } | ||
|
|
||
| func TestClusterAccessCredentialChanged(t *testing.T) { |
There was a problem hiding this comment.
The ClusterAccessCredentialChanged function in pkg/util/cluster.go currently does not check if SecretRef or ImpersonatorSecretRef has changed. If a user updates these references to point to a different secret, the function will incorrectly return false (assuming other fields like APIEndpoint are identical). This means credential changes might not be detected.
Consider updating ClusterAccessCredentialChanged in pkg/util/cluster.go to include these fields, and then add corresponding test cases here to verify that behavior.
There was a problem hiding this comment.
Pull request overview
Adds missing unit coverage for cluster readiness detection and access-credential change detection utilities, improving confidence in cluster lifecycle and controller behaviors that depend on these helpers.
Changes:
- Add table-driven unit tests for
IsClusterReadyacross key condition permutations. - Add table-driven unit tests for
ClusterAccessCredentialChangedacross each relevant field change scenario.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| Type: "SomeOtherCondition", | ||
| Status: metav1.ConditionTrue, | ||
| }, |
| { | ||
| name: "empty specs — no change", | ||
| newSpec: clusterv1alpha1.ClusterSpec{}, | ||
| oldSpec: clusterv1alpha1.ClusterSpec{}, | ||
| want: false, | ||
| }, |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7677 +/- ##
==========================================
+ Coverage 42.06% 42.07% +0.01%
==========================================
Files 879 879
Lines 54831 54831
==========================================
+ Hits 23063 23072 +9
+ Misses 30023 30014 -9
Partials 1745 1745
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What type of PR is this?
/kind feature
What this PR does / why we need it:
Two functions in
pkg/util/cluster.golacked unit test coverage. This PR adds tests for:IsClusterReady— covers no conditions,Ready=True,Ready=False,Ready=Unknown, and the case where theReadycondition is absent but other conditions are presentClusterAccessCredentialChanged— covers identical specs (returnsfalse), and each field that triggers a change:APIEndpoint,InsecureSkipTLSVerification,ProxyURL,ProxyHeader, plus the empty-specs base caseWhich issue(s) this PR fixes:
Fixes #7676
Special notes for your reviewer:
All new tests follow the table-driven pattern used throughout
cluster_test.go.Does this PR introduce a user-facing change?: